Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gateway: add Content-Location for non-default response formats #603

Merged
merged 3 commits into from
Apr 18, 2024

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Apr 16, 2024

Copy link

codecov bot commented Apr 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.57%. Comparing base (2f8a18d) to head (f96974f).

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #603      +/-   ##
==========================================
+ Coverage   59.72%   60.57%   +0.84%     
==========================================
  Files         238      238              
  Lines       29831    29462     -369     
==========================================
+ Hits        17817    17846      +29     
+ Misses      10405    10001     -404     
- Partials     1609     1615       +6     
Files Coverage Δ
gateway/gateway.go 83.54% <ø> (ø)
gateway/handler.go 79.62% <100.00%> (+1.34%) ⬆️
gateway/handler_car.go 79.89% <100.00%> (+1.28%) ⬆️
gateway/hostname.go 74.36% <100.00%> (+0.17%) ⬆️

... and 42 files with indirect coverage changes

@hacdias hacdias force-pushed the gateway-content-location branch 2 times, most recently from 92fc636 to de679ed Compare April 16, 2024 13:21
@hacdias
Copy link
Member Author

hacdias commented Apr 16, 2024

@lidel before I open a PR for the specifications, we should decide what to do with CAR requests that contain "parameters" in the Accept header itself (e.g. dups, order).

These cannot be translated into query URL parameters, so the Content-Location header would not be fully correct. For now, I am not adding it. I wonder if we shouldn't instead make it possible to also set these parameters through the URL, or just keep is as it is.

@hacdias hacdias self-assigned this Apr 16, 2024
@hacdias hacdias marked this pull request as ready for review April 16, 2024 13:25
@hacdias hacdias requested review from lidel and a team as code owners April 16, 2024 13:25
CHANGELOG.md Outdated Show resolved Hide resolved
gateway/handler.go Outdated Show resolved Hide resolved
gateway/handler.go Show resolved Hide resolved
gateway/handler.go Show resolved Hide resolved
gateway/handler.go Outdated Show resolved Hide resolved
@hacdias
Copy link
Member Author

hacdias commented Apr 17, 2024

@lidel I've opened a PR in the specs repository: ipfs/specs#471

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've reviewed ipfs/specs PRs, merged #604 into this one and pushed fix/test for case where format is different than Accept (91fdd90).

@hacdias If my changes look ok, feel free to merge, bubble up to rainbow, and bump our infra to rainbow image that includes this 🙏

@hacdias hacdias force-pushed the gateway-content-location branch from 91fdd90 to 0585641 Compare April 18, 2024 05:30
@hacdias hacdias force-pushed the gateway-content-location branch from 0585641 to f96974f Compare April 18, 2024 05:33
@hacdias hacdias enabled auto-merge (rebase) April 18, 2024 05:33
@hacdias hacdias merged commit eeea414 into main Apr 18, 2024
16 checks passed
@hacdias hacdias deleted the gateway-content-location branch April 18, 2024 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

gateway: fix car/block responses cached as deserialized ones when ?format=raw|car is missing
2 participants